Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support environent variables in config #1341

Merged
merged 11 commits into from
Jun 1, 2020

Conversation

7rulnik
Copy link
Contributor

@7rulnik 7rulnik commented May 11, 2020

What's the problem this PR addresses?

Lack of support of environment variables in config files #1146

How did you fix it?

Stringify yarnrc configs, split it by variables, replace with value from process.env, and fallback if it's not set.

I took fallback algorythm from docker-compose:

Added support for shell-style inline defaults in variable interpolation.
The supported forms are ${FOO-default} (fall back if FOO is unset) and
${FOO:-default} (fall back if FOO is unset or empty).

If you fine with this implementation I will add tests and will document this. Also I could rewrite it to deep-map and map strings only. It should be a better variant

Note that regexp named groups have limited support: http://kangax.github.io/compat-table/es2016plus/#test-RegExp_named_capture_groups. If this is a problem I can refactor it

@7rulnik 7rulnik force-pushed the env-variables-in-config branch 3 times, most recently from 52916c0 to fb5fbc7 Compare May 11, 2020 17:50
@arcanis
Copy link
Member

arcanis commented May 12, 2020

Hey! Thanks for tackling this. I think we wouldn't want to apply this as a pre-processing pass, otherwise it opens the door to pretty weird things if the environment variables contain things that could lead to valid YAML.

What do you think of instead doing this replacement within interpretValue, right here:
https://github.com/yarnpkg/berry/blob/master/packages/yarnpkg-core/sources/Configuration.ts#L434

@7rulnik
Copy link
Contributor Author

7rulnik commented May 12, 2020

Hmm, didn't see it. Will dig into it.
Btw, is there a way to run yarn with node --inspect flag?

@arcanis
Copy link
Member

arcanis commented May 13, 2020

easiest way is YARN_IGNORE_PATH=1 node --inspect-brk ./scripts/run-yarn.js

@7rulnik
Copy link
Contributor Author

7rulnik commented May 23, 2020

@arcanis I moved replacement into interpretValue, refactored replacement to .replace method, added tests and documented this.

Not sure about tests: maybe we should separate it and check all cases in packages/yarnpkg-parsers/tests/syml.test.js and in packages/yarnpkg-core/tests/Configuration.test.ts just check that replacement works?

Also there is something strange with the test on win.

@arcanis arcanis merged commit 5f58dc5 into yarnpkg:master Jun 1, 2020
@arcanis
Copy link
Member

arcanis commented Jun 1, 2020

Perfect! Thanks @7rulnik! 😃

@7rulnik 7rulnik deleted the env-variables-in-config branch June 1, 2020 14:23
@yinzara
Copy link

yinzara commented Sep 4, 2020

Could we have some way to simply disable this functionality all together maybe via .yarnrc.yml or via an environment variable?

@arcanis
Copy link
Member

arcanis commented Sep 4, 2020

Please open an issue with a repro. This PR is about the configuration, whereas you're talking about scripts 🙂

@paul-soporan
Copy link
Member

paul-soporan commented Sep 4, 2020

@yinzara Not sure what cross-env has to do with the feature introduced in this PR. This PR adds support for environment variables inside the configuration (.yarnrc.yml), so that the following can work:

npmRegistries:
  "//registry.npmjs.org":
    "npmAuthToken": ${MY_AUTH_TOKEN}

I believe you're referring to the environment variables inside our portable shell, which go through a completely different parser that correctly handles nested environment variables and a lot of other things. The portable shell has support for many features that work across all environments (including on Windows), like environment variables, command chains, subshells, some builtins.

For your use case, you can:

  • cross-env 'echo $PWD' - quote what needs to be passed to cross-env so that our shell doesn't expand it
  • echo $(pwd) - use a subshell and the pwd builtin which works across all environments

Edit: @arcanis responded faster than me. 😄

@yinzara
Copy link

yinzara commented Sep 4, 2020

THANKS! That's good enough! I appreciate it.

@paul-soporan
Copy link
Member

paul-soporan commented Sep 4, 2020

I guess we can also add support for the $PWD environment variable for better compliance with bash, even though we'd introduce 2 ways to do the same thing (echo $(pwd) and echo $PWD). @arcanis What do you think?

Edit: I realize that the discussion shouldn't continue here, my bad. 🤦

@yinzara
Copy link

yinzara commented Sep 4, 2020

I'll open another issue specifically to add $PWD resolution so it's bash compatible. I think that's an important thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants